Remove hard dependency on librarian #211
Conversation
Only try to use the librarian gem if the Cheffile is found (and `--no-librarian` is not specified). Complain to the user with instructions if the gem is not found. Also add a debug message is the Cheffile is not found.
|
This removes the hard dependency but there is still a problem in default value of |
|
@matschaffer could you review (or even merge, yikes!) this PR? I'll fix the mentioned bug in #221. |
|
Next Librarian version will extract librarian-chef to a separate gem. So we need to change the warning message at that time. |
librarian v0.1.0 will not include librarian-chef any more, as it will be extracted to own gem.
|
I was just about to submit a PR for loosening the librarian dependency. I have now librarian-chef 0.0.1.beta.1 installed which depends on librarian 0.1.0. This now conflicts with knife-solo 0.3.0.pre2 (depends librarian ~> 0.0.20) in my bundle. +1 for merging :-) |
|
Btw, I tested knife-solo yesterday also with librarian(-chef) betas and everything worked fine. So only things we need to change after their release is the warning message and development dependency requirement. |
|
@tknerr oh, and we could not just loosen the "librarian" gem dependency as 0.1.0 does not include all the classes we need. With librarian 0.1.0 the librarian-chef gem is needed. It is a good thing the previous releases locked the librarian requirement to 0.0.x. But obviously that can cause conflicts with other software. Bundler helps in many cases. |
|
@tmatilai fully agree! @matschaffer any chance of a .pre3 release once this is merged in? |
|
Hey guys, sorry just coming up for air after a massive b-day party for my son and mom. Should have some this week though. Any rough priority on the activity since yesterday would be appreciated. Either find me in the IRC channel or maybe just order them on http://huboard.com/matschaffer/knife-solo/board |
| if !File.exist? 'Cheffile' | ||
| Chef::Log.debug "Cheffile not found" | ||
| elsif !load_librarian | ||
| ui.warn [ |
There was a problem hiding this comment.
I'm a big fan of heredoc + margin removal for this sort of thing (e.g., https://github.com/matschaffer/knife-solo/blob/master/lib/knife-solo/info.rb#L7) Might be time just just pull in https://github.com/rubyworks/facets/blob/master/lib/core/facets/string/trim.rb and standardize on that.
There was a problem hiding this comment.
Ack. But I don't want newlines inside the strings that go to ui.warn, so I'll split the sentences to separare ui.warn calls.
There was a problem hiding this comment.
Ahh.... It didn't occur to me that that's what you were going for here. I
wonder if there's a cleaner approach. I'll ponder it for a bit.
-Mat
On Mar 25, 2013, at 17:15, Teemu Matilainen notifications@github.com
wrote:
In lib/chef/knife/solo_cook.rb:
@@ -133,10 +131,30 @@ def time(msg)
enddef librarian_install
return unless File.exist? 'Cheffile'ui.msg "Installing Librarian cookbooks..."Librarian::Action::Resolve.new(librarian_env).runLibrarian::Action::Install.new(librarian_env).runif !File.exist? 'Cheffile'Chef::Log.debug "Cheffile not found"elsif !load_librarianui.warn [
Ack. But I don't want newlines inside the strings that go to ui.warn, so
I'll split the sentences to separare ui.warn calls.
—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/211/files#r3521250
.
There was a problem hiding this comment.
The message construction was copied from another project doing a similar thing, so I had not spent too many seconds thinking how to do it. =)
The error message could also be improved further. Maybe something like:
Cheffile exists but Librarian-Chef could not be loaded.
Please add the librarian gem to your Gemfile or install it manually withgem install librarian.
Or you can skip Librarian and this warning by using--no-librarianoption or by puttingknife[:librarian] = falseto knife.rb.
But now off to bed...
|
Just some stylistic points. Looks good otherwise. |
|
Thanks a lot for reviewing! Really appreciate. I added the modifications as new commits. I'll merge this later if there are no new comments. |
Remove hard dependency on librarian
We should not require Librarian (nor Berkshelf) to be installed, but instead check their presence at run time.